Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Doc]Restructure monitoring docs to support new and legacy internal collectors #11714

Merged
merged 6 commits into from
Apr 14, 2020

Conversation

karenzone
Copy link
Contributor

@karenzone karenzone commented Mar 24, 2020

This PR starts with the goodness of #11586, pulls in doc architecture changes from #11614, and resolves recently introduced conflicts.

PREVIEW: http://logstash_11714.docs-preview.app.elstc.co/guide/en/logstash/master/configuring-logstash.html

@andsel
Copy link
Contributor

andsel commented Apr 9, 2020

LGTM

Copy link
Member

@robbavey robbavey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good so far! Couple of nitpicks and suggestions

`xpack.monitoring.collection.enabled` setting on Logstash. You must use the
`xpack.monitoring.enabled` setting to enable and disable data collection.
WARNING: Unlike for {es} and {kib} monitoring, there is no
`pack.monitoring.collection.enabled` setting on Logstash. You must use the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo

See <<monitoring-settings, Monitoring Settings>> for the list of settings.

If you don’t have an Elasticsearch output plugin configured in the pipelines,
add the setting `monitoring.cluster_uuid` to your logstash.yml.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to give an explanation of what this should be, and how they can retrieve it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how much we should invest in the legacy component as it is no longer one of the preferred approaches. Let's discuss.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, although we could just use the description from the newer mechanism

@@ -0,0 +1,152 @@
[role="xpack"]
[[monitoring-internal-collection-legacy]]
=== Collect {ls} monitoring data using legacy internal collectors
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a description of what is meant by 'legacy'?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have notes at key entry points directing users to one of the other options. What are you recommending?

Screen Shot 2020-04-09 at 3 56 14 PM

---------------------------------------------------

All data produced by {monitoring} for Logstash is indexed in the monitoring
All data produced by Logstash monitoring is indexed in the monitoring
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to verify whether this is still correct

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBD

This is particularly important to remember when you move from development
environments to production environments, where you often have dedicated
monitoring clusters.
IMPORTANT: When discussing security relative to the `elasticsearch` output,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is still correct as we don't route through production clusters any more

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBD


The universally unique identifier (UUID) for the monitoring cluster.
By default, {ls} identifies and uses the `cluster uuid` from the first
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It uses cluster_uuids from each of the elasticsearch outputs, rather than just the first


Optional settings that provide the paths to the Java keystore (JKS) to validate
the client’s certificate.

`xpack.monitoring.elasticsearch.ssl.keystore.password`::
`monitoring.elasticsearch.ssl.keystore.password`::

Optional settings that provide the password to the keystore.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also need to change verification_mode


Finds all {es} cluster nodes and adds them to the hosts list.

Defaults to `false`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need the newline here? It renders slightly weirdly
Screen Shot 2020-04-09 at 11 31 16 AM

docs/static/monitoring/collectors.asciidoc Show resolved Hide resolved
@karenzone karenzone force-pushed the monitoring-take3 branch 2 times, most recently from eb5670d to b11f6cb Compare April 14, 2020 14:35
These pieces live outside of the default Logstash pipeline in a dedicated
monitoring pipeline. This configuration ensures that all data and processing has
a minimal impact on ordinary Logstash processing. Existing Logstash features,
such as the <<plugins-outputs-elasticsearch,`elasticsearch` output>>, can be
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this comes from previously written copy - but I'm not sure what it is trying to say. I think it is trying to talk about the reuse of certain features from the elasticsearch output, such as retry, but I'm not 100%

@robbavey
Copy link
Member

@karenzone - I'm fine with committing this as is, and then work on the fine details in a subsequent PR

@karenzone
Copy link
Contributor Author

karenzone commented Apr 14, 2020

This PR is getting crushed under its own weight. I will merge what we have here, and immediately open a new one to track final comments.

Work continues in #11789

@karenzone karenzone merged commit 081ec78 into elastic:master Apr 14, 2020
@karenzone karenzone deleted the monitoring-take3 branch April 14, 2020 19:48
karenzone added a commit to karenzone/logstash that referenced this pull request Apr 14, 2020
…ollectors (elastic#11714)

* [Doc] added description of xpack.monitoring.collection.write_direct.enabled setting

* Added page to mark as deprecated the legacy internal collector and fixed all the `xpack.monitoring.*` references

* Included legacy collector file into monitoring overview

* Restructure monitoring docs

* Incorporate review comments

Co-authored-by: andsel <selva.andre@gmail.com>
elasticsearch-bot pushed a commit that referenced this pull request Apr 14, 2020
…tors (#11714)

* [Doc] added description of xpack.monitoring.collection.write_direct.enabled setting

* Added page to mark as deprecated the legacy internal collector and fixed all the `xpack.monitoring.*` references

* Included legacy collector file into monitoring overview

* Restructure monitoring docs

* Incorporate review comments

Co-authored-by: andsel <selva.andre@gmail.com>

Fixes #11787
elasticsearch-bot pushed a commit that referenced this pull request Apr 14, 2020
…tors (#11714)

* [Doc] added description of xpack.monitoring.collection.write_direct.enabled setting

* Added page to mark as deprecated the legacy internal collector and fixed all the `xpack.monitoring.*` references

* Included legacy collector file into monitoring overview

* Restructure monitoring docs

* Incorporate review comments

Co-authored-by: andsel <selva.andre@gmail.com>

Fixes #11787
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants